Remove PfC code and consolidate onto ImageCardComponent [WHIT-3983] - #11772
Conversation
4d63409 to
c38c4d9
Compare
c38c4d9 to
b1a78ed
Compare
GDSNewt
left a comment
There was a problem hiding this comment.
Didn't spot anything else other that the potential issue arising from hardcoding the html id 🤓
| <% unless (idx + 1) == current_images.size %> | ||
| <li aria-hidden="true"><hr class="app-view-edition-resource__section-break govuk-section-break govuk-section-break--visible"></li> | ||
| <% end %> | ||
| <ul id="uploaded_embeddable_image_list" class="govuk-list"> |
There was a problem hiding this comment.
If you had a document type config with
"usages": {
"example 1": {
"multiple": true,
},
"example 2": {
"multiple": true,
}
}
would it result in two divs ala
<div id="uploaded_embeddable_image_list">
</div>
...
<div id="uploaded_embeddable_image_list">
</div>
I don't think we have any config types writtenlike that currently, but could be an issue in future?
There was a problem hiding this comment.
Good spot - but yes, we currently don't have any like that (and I can't think of a situation where we'd have 'multiple' non-embeddable images in future 🤔 ) - but in any case this <div id is only used to help direct the tests to the relevant bit of the DOM.
This was added in 75c3f23, when EditionImagesController made multiple calls to `image_data.file.download!`. Those calls no longer exist.
This reverts commit a973a52. This functionality was set up to distinguish between image kinds on the old Plan For Change style Landing Pages, which were never migrated to the new ImageCardComponent way of doing things. The PfC pages have since been deleted. Any other document types that have multiple image kinds (e.g. Topical Events, with header, logo and embeddable Govspeak images) use the new ImageCardComponent pattern. So we can remove this unnecessary grouping in the "Uploaded images" section (which appears below where the ImageCardComponent kinds are rendered).
This reverts most of commit de4696f, which added some logic to include a horizontal rule that was only in force on the PfC images tab.
This was only used on the PfC pages, which have now been removed. It used to be that there was a shared image uploader for every kind of image, and the user had to specify what kind of image they were uploading in order to then get the correct validation etc on that image. Now, we have a dedicated image uploader for every image kind, so this branching logic is redundant.
We're going to switch to using the ImageCardComponent when rendering the "Uploaded images" section. One thing the current ImageComponent does that ImageCardComponent does not is render the markdown embed code, and also account for legacy editions where multiple images of the same filename may have been uploaded. In this commit we've copied over the tests and logic from the ImageComponent.
This previously returned 'image' for embeddable images, the logic for which was introduced in PR #11358. Looking at the PR, the embeddable image title never actually seemed to get used - or at least it no longer seems to be used in the current iteration of the code. When we swap out the "Uploaded images" section to use this title, we'll want to call it "Embeddable image" on the top of the card, otherwise we repeat "Image" in the card title followed by "Image" as the first row of data, which looks a bit odd.
The previous code required the 'status check' element to be a direct descendent of the 'image-processing-checker' data module container, but there's no reason why we can't have the 'status check' element nested several layers deep in the DOM. This straightforward swap-out of code allows for that.
This behaviour already exists on the cropping screen and on the specialised images area (e.g. Header logo), but general embeddable images still got stuck displaying "Processing" and required users to refresh the page.
We're pointlessly maintaining two different components for rendering a summary of an image within Whitehall. It means we're also duplicating logic around checking for uploaded state, showing a 'processing label', and so on. The long term aim has always been to converge onto a single 'cards' design, and the technical pay-off in being able to delete a redundant component and its suite of tests means it is now worth making that change. The card concept has been sufficiently bedded in, as it's been the way of expressing a document's "lead" image for many months now.
This was only used on the Images tab for "Uploaded images" (embeddable images). This has now been swapped out to use the image card component.
The ImageCardComponent is also used for Topical Event Header Image and Topical Event Logo, neither of which use the LeadImageCardComponent branch of logic in https://github.com/alphagov/whitehall/blob/e5f9e1bdbd5f852c9ff037808f97e42db780342e/app/views/admin/edition_images/index.html.erb#L24 This was causing the `<li>` to appear without any parent `<ul>` and therefore the browser's default 'bullets' to be rendered (normally suppressed with `govuk-list` class on the `<ul>`). There's no need for this to be an actual list in the markup - let's go with a div wrapping a div.
bcfec8e to
5aa82d7
Compare
| </template> | ||
|
|
||
| <%= render "govuk_publishing_components/components/summary_card", { | ||
| id: "uploaded_#{image_usage.key}_image_card", |
There was a problem hiding this comment.
Similar to #11772 (comment) - this id is only used for zoning in on a part of the DOM in the tests. It could in theory be repeated several times on a production page but there shouldn't be any real consequences to that.
What
Why
We're investigating reports of images intermittently rendering. Whilst investigating that, we uncovered a fair amount of avoidable image rendering complexity, which is paid down in this PR. It doesn't solve the original issue, but it gives us a cleaner base from which to investigate futher.
Jira: https://gov-uk.atlassian.net/browse/WHIT-3983
Screenshots
This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.